Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create the toolkit for plugin authors #88

Merged
merged 19 commits into from
Oct 15, 2024
Merged

Create the toolkit for plugin authors #88

merged 19 commits into from
Oct 15, 2024

Conversation

illright
Copy link
Member

@illright illright commented Aug 20, 2024 •
edited
Loading

Closes #71

This is a significant refactoring to the types and locations of utilities. Few things to note:

  • This PR adds a new package, @steiger/toolkit. It has these exports: 
    export type * from '@steiger/types'

export { findAllRecursively } from './find-all-recursively.js'
export { enableAllRules, createConfigs } from './create-configs.js'
export { createPlugin } from './create-plugin.js'
export type { ConfigObjectOf } from './config-object-of.js'

export { compareMessages, createFsMocks, joinFromRoot, parseIntoFolder } from './prepare-test.js'
    With it, the definition of the FSD plugin for Steiger looks like this: 
    const plugin = createPlugin({
  meta: {
    name: 'steiger-plugin-fsd',
    version: packageJson.version,
  },
  ruleDefinitions: [/* ... */],
})

const configs = createConfigs({
  recommended: enableAllRules(plugin),
})

export default {
  plugin,
  configs,
}

export type FSDConfigObject = ConfigObjectOf<typeof plugin>
  • I also fixed some type errors in Steiger core
  • The tests for these type utilities are currently missing. I tried to make them, but it's a whole separate story, and often you fight the testing framework more than actually testing. We will consider this in the future.

Issues to fix:

  • Context typings are beyond non-ergonomic, you have to type out the entire array of rules in the template args

Fixing it is beyond my power right now, I would like to at least get the baseline down. We don't use context currently, so once we start using it, then we can improve on this.

@illright illright marked this pull request as ready for review September 10, 2024 20:03
@illright
Copy link
Member Author

@daniilsapa could you review this please?

@illright illright mentioned this pull request Sep 10, 2024
Open
@daniilsapa
Copy link
Collaborator

@illright

Great job!
Most likely, we’ll need to add more utils in the future but you've prepared an excellent baseline.
I like that it has a solid foundation that covers 4 purposes:

  • Creating a plugin
  • Testing rules for a plugin
  • Creating configuration sets for a plugin
  • Utilities for custom rule code (findAllRecursively so far)

I really don't have any suggestions or complaints about it. I think it's good to go. 👍

Should we open an issue to create a short guide on how to build plugins for Steiger, so we remember that this task is still on the plate? Also, we could open an issue to cover the toolkit with tests as you mentioned that was a whole separate story.

@illright
Copy link
Member Author

Yep, I opened an issue for type tests already (#98), and I will also create an issue for the guide. I still don't fully know where is best to host these guides. For 1.0.0, we will definitely want a dedicated docs website, but for now we might use GitHub wiki.

@illright
Copy link
Member Author

I'll let you merge your PR first and then take care of the conflicts and merge this one after

daniilsapa
daniilsapa reviewed Sep 17, 2024
View reviewed changes
packages/toolkit/tsup.config.ts
treeshake: true,
clean: true,
esbuildOptions(options) {
options.define = { 'import.meta.vitest': 'undefined' }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I've been meaning to ask you for a while. Why do some functions have tests for them placed in the same file? Why can't we just put them in separate files and remove that redundancy of having to replace import.meta.vitest with undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just find it more convenient sometimes, especially when there's not a lot of code and not a lot of test cases. Having a separate *.spec.ts gets in the way of the file structure, so if I have a couple files, the duplicated names of test files are a bit annoying to deal with.

@illright
Copy link
Member Author

Okay, I merged the updates and cleaned everything up. Also tested it with the test cases from #71. @daniilsapa could you give it a final review please?

daniilsapa
daniilsapa reviewed Oct 12, 2024
View reviewed changes
packages/steiger/src/app.ts
@@ -65,7 +65,3 @@ export const linter = {
return [runRulesFx.doneData, () => watcher.close()] as const
},
}

export function defineConfig(config: Config) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great updates!

I have only one question about this thing.

Could you please explain why you decided to remove this function? I know it just returned the same object you passed it. This would cause errors like "The requested module does not provide an export named 'defineConfig'" and could be annoying for the users. And I know that the project is in the beta stage, so they should expect some APIs to change.

But in the future, it might happen that we need to do some pre-processing on the config. In fact, I was thinking the opposite, about adding some flag like "defined" for the config that went through defineConfig and considering it the only valid config. Yeah, and throw an exception if it doesn't have the flag, so people get used to wrapping their configs in defineConfig for the sake of some far-off new but quite possible features added to defineConfig.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think that I removed it by mistake, thanks for bringing that up. I wanted to beef up the type inference capabilities of this function, but gave up for the moment, and then just accidentally removed the whole thing.

But in the future, it might happen that we need to do some pre-processing on the config.

Nah, I would really prefer the defineConfig function to just be for typing. That seems to be the convention with other tools in the JS ecosystem, no one expects this function to be a requirement or to modify the config in any way. Besides, any processing we could do through this function could also be done in Steiger itself, without putting any burden on the user

I'll bring this function back

daniilsapa
daniilsapa approved these changes Oct 15, 2024
View reviewed changes
Copy link
Collaborator

@daniilsapa daniilsapa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any issues now. Thanks!

@illright illright merged commit 362a473 into master Oct 15, 2024
7 checks passed
@illright illright deleted the toolkit branch November 12, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniilsapa daniilsapa daniilsapa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement the type safety utilities for plugin authors
2 participants
@illright @daniilsapa